External subnets #177
External subnets #177
Conversation
FelixMcFelix
left a comment
FelixMcFelix
left a comment
There was a problem hiding this comment.
Thanks Nils, I've had a look over the P4 and the test machinery. I didn't see anything on the API side to give me pause.
| // Remove non-existent entry and be sure it fails | ||
| Ok(()) |
There was a problem hiding this comment.
The test was there, but the comment was in the wrong place.
| // On egress, we deencapsulate the geneve packet as we do for NAT, but we don't | ||
| // rewrite the source address before forwarding the packet. | ||
| async fn test_egress(switch: &Switch, test: &ExternalTest) -> TestResult { |
There was a problem hiding this comment.
This comment confused me a bit. The first 'we' refers to the switch, but the second 'we' is the test harness I'd assume (since sidecar.p4 just decaps on egress)?
There was a problem hiding this comment.
Yeah, that's fair. I think what's really confusing things here is that the diagram further down (which is taken from the RFD) tracks the packet from inside the instance to switch egress, while the test starts with a packet that has already been processed by OPTE. The comment was trying to explain why that the source IP address hasn't changed because there is no SNATing in play for these subnets. This is additionally opaque because, when reading from the top down, you haven't even seen the diagram yet.
I took that bit out altogether, since it's just answering a question nobody asked.
| // On ingress, we wrap the incoming packet with a geneve header and forward to | ||
| // the correct gimlet. Unlike NAT ingress, there is no manipulation of the | ||
| // target address. | ||
| async fn test_ingress(switch: &Switch, test: &ExternalTest) -> TestResult { |
There was a problem hiding this comment.
I don't think NatIngress adjusts any target address, other than the destination MAC (which external subnets do too).
There was a problem hiding this comment.
Yeah, this is another comment on OPTE's NAT handling, which doesn't really have anything to do with this test. I took it out.
| pkt_src_ip: "fa00:22::1".to_string(), | ||
| pkt_src_mac: "a1:a2:a3:a4:a5:a6".to_string(), | ||
| pkt_src_port: 3333, | ||
| pkt_dst_ip: "fc00:13::1".to_string(), | ||
| pkt_dst_mac: "b1:b2:b3:b4:b5:b6".to_string(), | ||
| pkt_dst_port: 4444, |
There was a problem hiding this comment.
src_ip and dst_ip seem swapped here wrt. the diagram, also placing pkt_src_ip outside of the external_subnet for egress.
(I think that sidecar.p4 will just blindly decap traffic directed at the switch address anyhow? Which would explain this test passing.)
There was a problem hiding this comment.
I think the src and dst IPs are correct, and match this diagram. They are different in the ingress and egress cases. That is, the endpoints' IP addresses don't change, but the src/dst polarity does.
I did have the external subnet wrong but, as you noted, that doesn't actually come into play in the egress case.
| meta.nat_ingress_tgt = target; | ||
| meta.nat_inner_mac = inner_mac; | ||
| meta.nat_geneve_vni = vni; | ||
| meta.encap_needed = true; |
There was a problem hiding this comment.
meta.encap_needed is always equal to meta.nat_ingress_hit. Can we remove nat_ingress_hit and update the last remaining reference it has in table service to use encap_needed instead?
rcgoodfellow
left a comment
rcgoodfellow
left a comment
There was a problem hiding this comment.
P4 code looks good. A note below on weirdness (to me) of putting subnet routing in the Filter control, but not a blocker.
dpd/p4/sidecar.p4
Outdated
| ipv6_ctr.count(); | ||
| } | ||
|
|
||
| action forward_extsub_v4_to(ipv6_addr_t target, mac_addr_t inner_mac, geneve_vni_t vni) { |
There was a problem hiding this comment.
Along the same lines as this comment. It still feels weird to me that this is in the formerly named Local now named Filter control. Routing to an attached subnet feels a lot closer to NatIngress and I would have anticipated seeing a new control like AttachedSubnetIngress.
Nieuwejaar
force-pushed
the
external_subnets
branch
from
1da0aa4 to
8c22ce5
Compare
I've added both Ry and Levon as reviewers. I'm particularly interested on Ry's thoughts on the P4 (which is implemented slightly differently on sidecar-lite) and Levon's input on the API, but I'll take any feedback you have to offer.